Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: improve error message for invalid data URL #37701

Merged
merged 1 commit into from
Apr 3, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 10, 2021

Fixes: #37647

Changes the error message when importing module with an unknown/unsupported MIME type from:

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected string to be returned for the "format" from the "loader getFormat" function but got type object.

to

TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "data:application/json,[]" has an unsupported MIME type "application/json"

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 10, 2021
@targos
Copy link
Member

targos commented Mar 12, 2021

@nodejs/modules

@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 12, 2021
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my 2¢. Not sure that error code has been used for this purpose historically.

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
@bmeck
Copy link
Member

bmeck commented Mar 12, 2021

CC: @ljharb since this mentions MIME to user

@bmeck
Copy link
Member

bmeck commented Mar 12, 2021

to clarify a bit, these should have been picked up and could have been generated in the past

@ljharb
Copy link
Member

ljharb commented Mar 12, 2021

Seems fine to me, since anyone using a data URL is already familiar with MIMEs (this is ofc not true for almost every other specifier category).

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

Ping @nodejs/modules for reviews.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from my outstanding comments (non-blocking), everything else looks pretty good to me.

I will be digging around this part of codebase in a few days, so I will report back if I find issues.

lib/internal/modules/esm/loader.js Show resolved Hide resolved
test/es-module/test-esm-invalid-data-urls.js Show resolved Hide resolved
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Unsupported MIME "text/plain" loading "data:text/plain,export default 0".

Also it could be nice if we could support the parent context for tracing where it comes from eg if it were import('data:...') in a module, it helps to know where the import expression is.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

@guybedford Agreed, although I think this would deserve its own PR to align with other ERR_INVALID_MODULE_SPECIFIER uses in core.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2021
@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

@aduh95 I do think it's important to move the "unsupported MIME" part to the beginning of the message though. This is because the data URL could be of any length, so that when scanning the error the user should be able to easily see the "unsupported MIME" part first. It also fixes that the current error isn't quite gramatically complete without a comma or colon or something type of break otherwise.

Alternatively to make it gramatically correct:

  • Use a full stop and a new sentence - The MIME type "..." is incorrect.
  • Use a comma / colon / dash.
  • Use "has an unsupported MIME type "...""

@nodejs-github-bot
Copy link
Collaborator

@DerekNonGeneric DerekNonGeneric added the question Issues that look for answers. label Apr 2, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

I went to the last suggestion as it was the easiest to implement. I think you have a point regarding very long specifiers occulting the reason, maybe we can fix that in a similar way as proposed in #37852 (comment)?

@DerekNonGeneric
Copy link
Contributor

[…] maybe we can fix that in a similar way as proposed in #37852 (comment)?

I believe the feature request outlined in #37581 is precisely the error property we need.

@nodejs-github-bot
Copy link
Collaborator

Fixes: nodejs#37647

PR-URL: nodejs#37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 3, 2021

Landed in 46062a0

@aduh95 aduh95 merged commit 46062a0 into nodejs:master Apr 3, 2021
@aduh95 aduh95 deleted the invalid-mime-data-url branch April 3, 2021 18:06
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. question Issues that look for answers. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import from JSON data URL doesn't work
7 participants